Conversation
B1: Remove duplicated authorization code (BAPScope, ScopeProfiles,
MethodScopes, hasScope, parseScopes, createAuthorizationError) from
server-playwright and import from @browseragentprotocol/protocol.
B2: Add Zod input validation schemas for the 10 most critical MCP tool
arguments (navigate, click, type, fill, press, select, hover, element,
activate_page, extract) to catch invalid inputs before they reach the
BAP client.
B3: Fix token not refreshed on reconnect in BAPClient. Store the token
separately from the URL in WebSocketTransport, reconstruct the URL with
the current token on each connect/reconnect, and add updateToken()
method to both WebSocketTransport and BAPClient.
B4: Replace inline require('crypto') and require('fs') in ESM module
server-playwright/server.ts with top-level imports using node: protocol.
|
Closing this in favor of #8. The release-prep commit on now folds in the remaining blocking fixes from this PR as well: shared protocol authorization in server-playwright, MCP argument validation for critical tools, client token refresh on reconnect, and the ESM import cleanup. The branch has been re-verified locally with ERR_PNPM_NO_IMPORTER_MANIFEST_FOUND No package.json (or package.yaml, or package.json5) was found in "/Users/piyush/GitHub".. |
|
Superseded by #8. The release-prep commit on feat/webmcp-support folds in the remaining blocking fixes from this PR as well: shared protocol authorization in server-playwright, MCP argument validation for critical tools, client token refresh on reconnect, and the ESM import cleanup. I re-verified the branch locally with pnpm release:verify before closing this. |
1 participant
Summary
Fixes all 4 blocking issues identified in code review:
B1: Remove duplicated authorization code from server-playwright --
BAPScope,ScopeProfiles,MethodScopes,hasScope(),parseScopes(), andcreateAuthorizationError()were defined inline inserver-playwright/server.tsinstead of being imported from@browseragentprotocol/protocolwhere the canonical definitions live. Removed ~120 lines of duplicated code and replaced with imports.B2: Add Zod input validation on MCP tool arguments -- Tool arguments were cast with
as string/as numberwithout validation. Added Zod validation schemas for the 10 most critical tools (navigate,click,type,fill,press,select,hover,element,activate_page,extract). Invalid arguments now throw descriptive errors before reaching the BAP client.B3: Fix token not refreshed on reconnect -- The auth token was baked into the WebSocket URL at construction time and never updated on reconnect. Refactored
WebSocketTransportto store the base URL and token separately, rebuilding the URL with the current token on eachconnect()call. AddedupdateToken()method to bothWebSocketTransportandBAPClient.B4: Replace
require()in ESM modules --require('crypto')andrequire('fs')were used inline inserver-playwright/server.ts(an ESM module). Replaced with top-levelimport { timingSafeEqual } from "node:crypto"andimport fs from "node:fs".Changes
packages/server-playwright/src/server.tsrequire()with top-level ESM imports.packages/mcp/src/index.tsvalidateArgs()helper. Applied validation to 10 tool handlers.packages/client/src/index.tsWebSocketTransportto store token separately from URL. AddedupdateToken()to both transport and client.Test plan
pnpm build-- all 7 packages build successfullypnpm typecheck-- all packages pass type checkingpnpm test-- all 420 tests pass (163 protocol + 78 client + 66 mcp + 43 server-playwright + 70 cli)updateToken()works across reconnection